feat: restore focus after closing modal #1735
Conversation
chenxsan
requested review from
shortcuts and
francoischalifour
as code owners
✅ Deploy Preview for docsearch canceled.
|
|
is there an easy way for me to check this live without having to work out how to run it locally? from looking over the code itself, this looks correct though. |
|
Thanks a lot for the PR!
For a concrete test you'd have to run de playground locally, it should only require installing deps and run |
I can see there's a netlify deploy preview, but it's canceled, any chance to make it work so anyone can test it online instead of running it locally? |
Sure, I'll add one. |
You can remove the ignore build command of the netlify.toml file, but I don't know if we have a button to re-focus on the website. |
|
Converted into draft until I figure out the cypress test here. |
|
@shortcuts Seems we can't test this feature against the website as docusaurus customizes its search function based on Hence I would suggest running cypress against the example site, either |
FYI, if you're going to run locally, you would need Node 16. 18 won't work as far as I see. Here's how you would run the project locally:
|
This reverts commit 730f271.
Makes a lot of sense indeed! Changing the |
|
(again thanks a lot for the time spent here!) |
It's my pleasure to improve a project I use every day :) I think it's ready for review now, I've added some comments to explain why I made such change. |
|
Ooops, CI failed due to https://support.circleci.com/hc/en-us/articles/115014359648-Exit-Code-137-Out-of-Memory |
oops D: you can update https://github.com/algolia/docsearch/blob/main/package.json#L18 to prevent building the playground, we don't need it on the CI anyway I think adding |
| } | ||
|
|
||
| export function DocSearch(props: DocSearchProps) { | ||
| const searchButtonRef = React.useRef<HTMLButtonElement>(null); | ||
| const activeElementRef = React.useRef<Element>(document.body); |
There was a problem hiding this comment.
Is this working in server environments where document isn't defined?
There was a problem hiding this comment.
Interesting, I've never thought of rendering DocSearch in server side.
I guess it won't work.
Any suggestion?
There was a problem hiding this comment.
How about this:
I've reran the tests locally, seems fine, though we need to apply type assertion for the initial object.
There was a problem hiding this comment.
Isn't the effect covered by the onOpen() callback?
I think we better have activeElementRef accept null as initial value, and update its value in the onOpen() and onClose() callbacks. This way, the effect can also be removed, and there's no document in the initial render path.
export function DocSearch(props: DocSearchProps) {
const searchButtonRef = React.useRef<HTMLButtonElement>(null);
+ const activeElementRef = React.useRef<Element | null>(null);
const [isOpen, setIsOpen] = React.useState(false);
const [initialQuery, setInitialQuery] = React.useState<string | undefined>(
props?.initialQuery || undefined
);
const onOpen = React.useCallback(() => {
setIsOpen(true);
+ activeElementRef.current = document.activeElement || document.body;
}, [setIsOpen]);
const onClose = React.useCallback(() => {
setIsOpen(false);
+
+ if (activeElementRef.current) {
+ activeElementRef.current.focus();
+ activeElementRef.current = null;
+ }
}, [setIsOpen]);
- React.useEffect(() => {
- if (
- !isOpen &&
- activeElementRef.current &&
- activeElementRef.current instanceof HTMLElement
- ) {
- activeElementRef.current.focus();
- }
- }, [isOpen]);
// ...
}There was a problem hiding this comment.
Unfortunately this won't work,
const onClose = React.useCallback(() => {
setIsOpen(false);
+
+ if (activeElementRef.current) {
+ activeElementRef.current.focus();
+ activeElementRef.current = null;
+ }
}, [setIsOpen]);
I believe I tried this in the beginning, and it would cause two tests to fail:
I.e., clicking the button to open dialog, then invoking shortcut to close the dialog would fail. I haven't yet figured out what could be wrong. Guess operating DOM right after calling setIsOpen(false) doesn't fit well with React. Introducing another useEffect would let React schedule the jobs.
Related issue #1370
cc @patrickhlauke if you don't mind please help review this one.
Basically the code would store the
document.activeElementwhen modal is opened, and callfocuson that storeddocument.activeElementwhen modal is closed.I've added a button to the examples so we can test if the focus can be retained.